Skip to content

Update TreeAdapter to work with const inputs#1775

Merged
danrbailey merged 3 commits intoAcademySoftwareFoundation:masterfrom
danrbailey:tree_adapter_improvements
Sep 18, 2024
Merged

Update TreeAdapter to work with const inputs#1775
danrbailey merged 3 commits intoAcademySoftwareFoundation:masterfrom
danrbailey:tree_adapter_improvements

Conversation

@danrbailey
Copy link
Contributor

@danrbailey danrbailey commented Mar 12, 2024

The TreeAdapter has a few problems, most notably related to the use of the tree()/constTree() methods. It appears that the TreeAdapter is intended to work with const inputs (because some of the typedefs remove const-ness) so it seems counter-intuitive that these methods do not always work for const inputs. In addition, the resulting error is confusing because the compiler complains about duplicate methods being defined.

I created a bunch of unit tests to validate against what I considered acceptable input and modified the TreeAdapter to fix these issues.

The one area where I extended it was to explicitly add support for TreeAdapter<const Grid<TreeT>>. This is very useful when const-ness is baked into an input template argument and seemed consistent with supporting TreeAdapter<const TreeT>.

All of the rest of the unit tests pass and none of these changes are expected to affect compatibility.

(@kmuseth)

@danrbailey
Update TreeAdapter to work correctly with all const arguments and add…
5862e8d 
… extensive unit tests

Signed-off-by: Dan Bailey <danbailey@ilm.com>
@danrbailey danrbailey requested a review from kmuseth as a code owner March 12, 2024 05:59
@danrbailey
Use non-const Grid in const version of type trait
6b9a6c2 
Signed-off-by: Dan Bailey <danbailey@ilm.com>
@danrbailey danrbailey mentioned this pull request Mar 14, 2024
Draft
@Idclip
Copy link
Contributor

Idclip commented Mar 18, 2024

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

@danrbailey
Copy link
Contributor Author

danrbailey commented Mar 18, 2024

What happens with an adapter templated on a const ValueAccessor? Or rather, is that behaviour still the same before and after this change

The behavior before is that it fails to compile, the behavior after is that it compiles successfully. I attached a unit test with these fixes, running the unit test without the fixes results in compilation errors on these lines - 544, 546, 550, 573, 575, 577, 589, 594, 595, 604, 605, 608, 610, 611, 612, 614.

Here's one example related to your question:

FloatGrid floatGrid;
TreeAdapter<tree::ValueAccessor<const FloatTree>>::tree(floatGrid);

I would expect this to work, but it results in this compile error:

openvdb/unittest/TestGrid.cc:604:38: error: no matching function for call to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::tree(openvdb::v11_0::FloatGrid&)'
  604 |         AdapterConstT::tree(floatGrid);
      |                                      ^
In file included from openvdb/openvdb.h:13,
                 from unittest/TestGrid.cc:5:
openvdb/Grid.h:1135:22: note: candidate: 'static openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType& openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::tree(openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType&) [with _TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >; openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<_TreeType, true, void, typename std::decay<decltype (make_index_sequence_impl<(std::max((unsigned int)(1), _TreeType::DEPTH) - 1)>())>::type> >::TreeType = const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >]'
 1135 |     static TreeType& tree(TreeType& t) { return t; }
openvdb/Grid.h:1135:37: note:   no known conversion for argument 1 from 'openvdb::v11_0::FloatGrid' {aka 'openvdb::v11_0::Grid<openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > > >'} to 'openvdb::v11_0::TreeAdapter<openvdb::v11_0::tree::ValueAccessorImpl<const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >, true, void, openvdb::v11_0::index_sequence<0, 1, 2> > >::TreeType&' {aka 'const openvdb::v11_0::tree::Tree<openvdb::v11_0::tree::RootNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::InternalNode<openvdb::v11_0::tree::LeafNode<float, 3>, 4>, 5> > >&'}
 1135 |     static TreeType& tree(TreeType& t) { return t; }

To extract the problematic portion of the partial specialization:

template<typename _TreeType>
struct TreeAdapter<tree::ValueAccessor<_TreeType> >
{
    using TreeType             = _TreeType;
    ....
    using GridType             = Grid<TreeType>;
    ....

    ....
    static TreeType& tree(GridType& g) { return g.tree(); }
    ....
}

You can see that calling this with TreeAdapter<tree::ValueAccessor<const FloatTree>> results in tree(Grid<const FloatTree>) but no tree(Grid<FloatTree>&).

This is not expected to change behavior for any method which does instantiate correctly, just fill in all the missing instantiations.

@danrbailey danrbailey merged commit 3b473dd into AcademySoftwareFoundation:master Sep 18, 2024
@danrbailey danrbailey deleted the tree_adapter_improvements branch September 18, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmlait jmlait jmlait approved these changes

@kmuseth kmuseth Awaiting requested review from kmuseth kmuseth is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@danrbailey @Idclip @jmlait